-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: always show cursor after control+c #2635
Conversation
I remember we explicitly removed ctrl+c handler at some point. @tdejager do you remember why? |
We explicitly removed the CTRL+C handler in the |
Also you can use |
… it and the gnu version is not guaranteed to work.
…rget for it and the gnu version is not guaranteed to work." This reverts commit b47ee5c.
src/cli/run.rs
Outdated
@@ -326,6 +326,14 @@ fn disambiguate_task_interactive<'p>( | |||
..ColorfulTheme::default() | |||
}; | |||
|
|||
// Ignore CTRL+C in the dialoguer prompt so that it shows the cursor again. | |||
ctrlc::set_handler(move || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still in favor of using tokio if we can make it work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it back
I notice one weird thing with this change. It seems to work like I would expect when killing things run after this command. But canceling an install acts differently from cancelling a normal install: This is repeatable for some reason, @wolfv or @baszalmstra would you know why that could happen? e.g. did I just introduce a terrible bug that my testing isn't verifying? |
e6efdfd
to
cb928fc
Compare
This PR is ready for review. I tried to react on ctrl-c via tokio as suggested by @wolfv, but I couldn't get it to work. In order to reset the cursor, one has to write to stdout, and as far as I can tell no print goes through in that context. When using the |
I retested it and it did indeed fix the issues I had. So I'll merge it. |
Interesting. Even with But - whatever works! :) |
I didn't try flushing, but since stdout is line-buffered, it should still have worked with |
Not sure if that is still true when the terminal is e.g. set into |
Mmmh interesting, will try flushing tomorrow |
Flushing doesn't help, unfortunately, let's stick with the |
fixes #1548 #2625
The issue is related to and fixed with help of console-rs/dialoguer#77